Skip to content
This repository was archived by the owner on Oct 8, 2021. It is now read-only.

Minor performance optimization for closestEnabledButton in buttonMarkup#3122

Merged
jblas merged 1 commit intojquery-archive:masterfrom
hpbuniat:closestEnabledButton-speedup
Nov 22, 2011
Merged

Minor performance optimization for closestEnabledButton in buttonMarkup#3122
jblas merged 1 commit intojquery-archive:masterfrom
hpbuniat:closestEnabledButton-speedup

Conversation

@hpbuniat
Copy link
Contributor

I changed the $.inArray to indexOf - results at http://jsperf.com/fsdsgt4.
The biggest impact comes with real mouse usage, as the vmouseout/vmouseover events are fired very often.

@jblas
Copy link
Contributor

jblas commented Nov 21, 2011

@hpbuniat @eddiemonge @Wilto

And so we come full-circle, almost all the way back to what the original implementation used, which used $.fn.hasClass() which basically does a string indexOf() under the hood which this pull request is proposing.

The one thing we were trying to do with hasClass() was make sure we didn't get any false positives since some of the button hierarchies built up use classes like ui-btn, ui-btn-inner, ui-btn-text, etc.

It would be interesting to weigh the original hasClass() against the current 2 in the jsperf above.

@eddiemonge
Copy link
Contributor

What was the reason we switched to inArray instead of indexOf? Wasnt it something about IE support?

@jblas
Copy link
Contributor

jblas commented Nov 21, 2011

@eddiemonge

You switched it from $.fn.hasClass() to using split() and Array.indexOf() here:

b0db897#js/jquery.mobile.buttonMarkup.js

which was a problem because IE7 (WP7) didn't support Array.indexOf so @Wilto switched things to $.inArray():

410a169#js/jquery.mobile.buttonMarkup.js

@Wilto
Copy link
Contributor

Wilto commented Nov 21, 2011

Yeah—calling indexOf on an array causes Internet Explorer to go stomping off to its room, slamming doors and yelling about how “when it turns eighteen it is so out of here .”

…That was a strained metaphor, but you get what I’m saying: using indexOf on an array breaks IE 6/7/8.

@eddiemonge
Copy link
Contributor

As long as IE7 likes indexOf with strings (and other browsers too of course), I think this PR looks good.

jblas added a commit that referenced this pull request Nov 22, 2011
Minor performance optimization for closestEnabledButton in buttonMarkup
@jblas jblas merged commit 2b40784 into jquery-archive:master Nov 22, 2011
@jblas
Copy link
Contributor

jblas commented Nov 22, 2011

@hpbuniat thanks for the patch.

@eddiemonge
Copy link
Contributor

One thing i thought of that i should ask, is there ever a time when ui-btn gets prefixed?

@hpbuniat
Copy link
Contributor Author

I'm glad i could help.

@eddiemonge, i've counted 59 occurrences of ui-btn in the code, none is prefixed. There is also no css that matches to /[^\.]ui-btn/.

@jblas
Copy link
Contributor

jblas commented Nov 22, 2011

@hpbuniat is correct, we don't namespace/prefix ui-* attributes. That is, all of our CSS class names start with ui- and nothing before that.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants